Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace .sr-only with .visually-hidden, use static Helper.screenReaderOnly class where applicable, fix SpinnerAjax(Button|Link) label rendering #1010

Conversation

vrozkovec
Copy link
Contributor

In current implementation, just calling setLabel() triggers AttributeModifier that adds .sr-only even though the label's model may not be empty and in some setups label is not rendered because .sr-only class hides it.

With this change, the check is made in onConfigure, but not very nice get("label") call is used - better way would require BootstrapAjaxLink and BootstrapAjaxButton methods that return label to be protected instead of private. Or extracting label's id to the constant and making protected that.

@vrozkovec vrozkovec force-pushed the wicket-10.x-bootstrap-5.x-spinner branch from 0a52cd8 to b1e3d40 Compare December 3, 2023 23:16
@reckart
Copy link
Contributor

reckart commented Dec 4, 2023

If I remember correctly, in BS5, sr-only has been replaced: https://getbootstrap.com/docs/5.3/getting-started/accessibility/#visually-hidden-content

@vrozkovec
Copy link
Contributor Author

Right, I've missed that. I've slightly enlarged scope of this PR, replacing .sr-only with .visually-hidden and using static Helper, where applicable.

@vrozkovec vrozkovec changed the title Only add class .sr-only when the label is empty - check in onConfigure Replace .sr-only with .visually-hidden, use static Helper.screenReaderOnly class where applicable, fix SpinnerAjax(Button|Link) label rendering Dec 4, 2023
@vrozkovec
Copy link
Contributor Author

All done

@martin-g martin-g merged commit a0c58cf into martin-g:wicket-10.x-bootstrap-5.x Dec 5, 2023
2 checks passed
@martin-g
Copy link
Owner

martin-g commented Dec 5, 2023

Thank you, @vrozkovec !

@martin-g
Copy link
Owner

martin-g commented Dec 5, 2023

Should we backport it to wicket-9.x-bootstrap-5.x ?

@solomax
Copy link
Contributor

solomax commented Dec 5, 2023

Should we backport it to wicket-9.x-bootstrap-5.x ?

yes, please :)

martin-g pushed a commit that referenced this pull request Dec 5, 2023
…rOnly class where applicable, fix SpinnerAjax(Button|Link) label rendering (#1010)

* Only add class .sr-only when the label is empty - check in onConfigure

* Update helper class name for screenReaderOnly content

* Replace .sr-only -> .visually-hidden, use static Helper where applicable

* rename all references from "srOnly" to "visuallyHidden" for consistency

(cherry picked from commit a0c58cf)
@martin-g
Copy link
Owner

martin-g commented Dec 5, 2023

Done!

@vrozkovec vrozkovec deleted the wicket-10.x-bootstrap-5.x-spinner branch December 5, 2023 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants